-
Notifications
You must be signed in to change notification settings - Fork 241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove MultiProgress::join() based on #231 #284
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this! In order to make sure I can review this closely, I'm asking for some changes to the commit history presented in this PR. I usually use git's interactive rebase to do that, but if you're unfamiliar with that stuff I can also help split it up more. The goal is to have a few commits that each independently could build and pass the tests.
Also, I think the reference to #116 in the second commit's message is wrong, can you fix that up? It'd be nice if you could shorten the first line of that message to fit within 70-80 chars while you're in there, and a high level description of what problem the commit is trying to fix and the strategy for doing so.
9a01750
to
d1a2404
Compare
6ee4a8a
to
120d652
Compare
I don't know enough about indicatif to know why it causes the defect, but the current PR does not respect when a progress bar is hidden.
Corresponding output:
|
@boc-tothefuture are you sure this PR actually changed this behavior? See the docs for |
You are correct, the same behavior exists in the currently released version. I did not think I was experiencing that until I switched. My apologies. |
a179348
to
47a7427
Compare
Except for the typos, this is looking pretty good! Thanks for working through my comments. |
c031f70
to
de129f1
Compare
Typos fixed also fixed the failing CI |
I just pushed another test that calls |
9ef6584
to
de129f1
Compare
Instead of drawing from a thread blocked on MultiProgress::join(), draw directly from the threads updating the ProgressBars in the MultiProgress, using write access to MultiProgress's state to synchronize terminal updates.
The leaky bucket makes sure that progress bars are drawn on average a the desired rate but allows it to burst faster if progress is not uniform. It works by having a "bucket" of redraws that are added to every time a draw is requested, when the bucket is full the redraws are skipped, for every tick a redraw is removed from the bucket. This is to fix the regression of console-rs#166 when join was removed.
de129f1
to
fee4bdc
Compare
Rebased on the the latest main (ignore the moment of stupidity where I rebase it on master instead). Thank for all your help an patience with this. |
This change moves drawing of MultiProgress from the join() thread to the child ProgressBar updates. fixes #125, #33 and lay groud work for work on #205
It is backwards-incompatible.
It is based on the work of @marienz in #231 more discussion and detail on that pull request.